Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default to packed for repeated primitives in proto3. #707

Merged
merged 4 commits into from
Jun 16, 2024

Conversation

rcorre
Copy link
Contributor

@rcorre rcorre commented Jan 17, 2024

Fixes #704.
Fixes #706.

I wasn't sure how to add tests for either.
For #704, I didn't see a way to distinguish which syntax was being used in a test.
For #706, I didn't see a way to have an invalid proto in a test (the parser fails before you get to the test).

I'm not sure I handled the error correctly, the output looks like:

--- stderr
codegen failed: [packed = true] can only be specified for repeated primitive fields

As opposed to other errors, that have multiple layers:

--- stderr
codegen failed: parse and typecheck

Caused by:
    0: using pure parser
    1: error in `api.proto`: object is not found by path `tring` in scope `.api.Example`
    2: object is not found by path `tring` in scope `.api.Example`

There's also no line/column number, but I'm not sure if that's expected.

@stepancheg
Copy link
Owner

Merged. Thanks!

I wasn't sure how to add tests for either.

Add a test to test crate which adds a proto3 message with repeated primitive, and checks that serialized as packed.

However, given that the crate is in low maintenance mode, I accept this as is.

I didn't see a way to distinguish which syntax was being used in a test.

Test codegen can be modified to emit this information. Or proto version can be obtained from reflective API.

I didn't see a way to have an invalid proto in a test

I think the project does not have a lot of tests for errors.

There's also no line/column number, but I'm not sure if that's expected.

When the crate was implemented, I did not know how to do errors properly. Now I know, but as I said, crate is in low maintenance.

@stepancheg stepancheg closed this Feb 25, 2024
@stepancheg stepancheg reopened this Feb 25, 2024
@stepancheg
Copy link
Owner

Wait, I'm reverting this commit. I believe it is implemented incorrectly. I suspect the problem is in pure rust proto parser. I suspect protoc-based parser populates this field, but pure parser is not:

field.field.proto().options.get_or_default().packed()

Also, let's add a test.

@rcorre
Copy link
Contributor Author

rcorre commented Mar 3, 2024

Thanks for the review! I've added a test. I don't follow what you're saying about the pure parser not setting the packed field. I think the pure parser handles options here, and the existing tests show that packed=true/packed=false work for both protoc and pure.
Was there a bug you saw when this merged?

@stepancheg
Copy link
Owner

stepancheg commented Mar 7, 2024

I don't follow what you're saying about the pure parser not setting the packed field

I suspect protoc parser already sets this field by default, and pure parser does not.

https://github.com/protocolbuffers/protobuf/blob/94ab406bcf048146ac494335e3b8ad9ef22fdabb/src/google/protobuf/descriptor.proto#L661

So my hypothesis is, we need to patch pure parser instead of patching codegen.

@rcorre
Copy link
Contributor Author

rcorre commented Mar 7, 2024

$ protoc test-crates/protobuf-codegen-protoc-test/src/common/v2/test_repeated_packed_pb.proto -I test-crates/protobuf-codegen-protoc-test/src/common/v2/ -o /tmp/descriptor
$ protoc --decode=google.protobuf.FileDescriptorSet google-protobuf-all-protos/protobuf/protobuf-git/src/google/protobuf/descriptor.proto  -I google-protobuf-all-protos/protobuf/protobuf-git/src/google/protobuf  < /tmp/descriptor > /tmp/descriptor.txt

descriptor.txt

That looks to me like protoc is not setting the field by default, which makes sense, since the field is optional:

  message_type {
    name: "TestIssue281"
    field {
      name: "values"
      number: 1
      label: LABEL_REPEATED
      type: TYPE_FIXED32
      options {
        packed: true
      }
      json_name: "values"
    }
  }
  message_type {
    name: "TestPackedDefault"
    field {
      name: "varints"
      number: 1
      label: LABEL_REPEATED
      type: TYPE_UINT32
      json_name: "varints"
    }
  }

So I think both parsers treat packed as optional, and the codegen needs to handle the field not being set.

@stepancheg stepancheg merged commit 5d8bdb4 into stepancheg:master Jun 16, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants